Conversation
The synthetic_id cookie was missing the HttpOnly attribute, allowing any XSS on a publisher page to exfiltrate the tracking identifier via document.cookie. HttpOnly is safe to set because integrations receive the synthetic ID via the x-synthetic-id response header and no client-side JS reads it from the cookie directly. Also documents the rationale for each security attribute (Secure, HttpOnly, SameSite=Lax, Max-Age) in the doc comment, and adds debug_assert guards against cookie metacharacter injection in both the synthetic_id value and cookie_domain. Closes #411
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-scoped security hardening. The HttpOnly addition is correct, the doc comment is excellent, and the change is minimal. One concern about the debug_assert! approach and a few smaller items below.
0 Blockers · 1 High · 3 Medium
| /// - `SameSite=Lax`: sent on same-site requests and top-level cross-site navigations. | ||
| /// `Strict` is intentionally avoided — it would suppress the cookie on the first | ||
| /// request when a user arrives from an external page, breaking first-visit attribution. | ||
| /// - `Max-Age`: 1 year retention. |
There was a problem hiding this comment.
P2 — Missing # Panics doc section
Per project conventions, functions that can panic should document it. The debug_assert! macros below will panic in debug/test builds. Consider adding:
/// # Panics
///
/// Panics in debug builds if `synthetic_id` or `cookie_domain` contains
/// cookie metacharacters (`;`, `=`, `\n`, `\r`).
There was a problem hiding this comment.
@prk-Jr sorry the previous review didn't have clear suggestions.
Solid, well-scoped security fix. The HttpOnly addition is correct and the doc comment is excellent. Three actionable findings — two about the sanitization approach, one about the assert!.
2 participants
Summary
synthetic_idcookie was missingHttpOnly, allowing XSS on a publisher page to exfiltrate the tracking identifier viadocument.cookie.HttpOnlyis safe to add because no client-side JS reads this cookie — integrations receive the synthetic ID via thex-synthetic-idresponse header instead.debug_assert!guards against cookie metacharacter injection in both thesynthetic_idvalue andcookie_domain, and documents the rationale for each security attribute in the function's doc comment.Changes
crates/common/src/cookies.rsHttpOnlyto cookie format string; adddebug_assert!metacharacter guards; update doc comment to enumerate all security attributes with rationaleCloses
Closes #411
Test plan
cargo test --workspacecargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest run— pre-existing ESM/CJS failure onmain, unrelated to this changecd crates/js/lib && npm run formatcd docs && npm run formatcargo build --bin trusted-server-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)